jenkins test this again please

Tests are all fixed up now. :)

@yaauie and @ycombinator This is ready for a re-review please.

Looking at the response from the API (using this PR), I see this structure:

{
  ...,
  "pipelines": {
    "main": {
      "graph": {
        "hash": "...",
        "type": "lir",
        "version": "...",
        "graph": {
           "vertices": { ... },
           "edges": { ... }
         }
       }
     }
  }
}

How difficult would it be to rename the outer graph key to representation instead? That would match the example JSON shown in the issue description. If it's not trivial to do, we can rename it on the consumer (beats) side.

@ycombinator Sadly, this isn't trivial. I think it would be better to change this on the beats side.

@cachedout I just noticed that while this PR does indeed implement the requirements of #10119 (to expose additional fields for a pipeline via the GET _node/pipelines/:id API), it has the side effect of exposing the additional pipeline fields in GET _node/pipelines as well, for every pipeline listed in that API's response.

Personally I'm okay with that but it could potentially make the API response quite heavy, if there are several pipelines being run on a node. I'm not sure if it would be desirable to keep the GET _node/pipelines API leaner (as it is currently). Thoughts?

jenkins, test this

Hi @yaauie and @jsvd. We're about ready to start getting these merged. Would you mind please reviewing this when you have a moment? It will help our team stay on track. Thanks!

@cachedout I just noticed this point in a comment from @jsvd on the issue for this PR:

the representation is only returned if the query parameter ?representation=true is passed

If its not a lot of work, could we implement this as part of this PR as well?

I tested this PR along with logstash-plugins/logstash-output-elasticsearch#857, and all works as expected. The pipelines.{id}.cluster_uuids field in the GET _node/pipelines/{id} response shows the expected Elasticsearch cluster UUID(s).

Having cluster_uuids as a top-level field (within a pipeline object) is certainly very convenient from a Metricbeat consumption/parsing POV. However, purely from a data model POV, it feels a bit hacky; perhaps the field should only show up inside pipelines.{id}.graph.graph.vertices objects that represent Elasticsearch output plugin vertices? And even within each such vertex object, the cluster_uuid field should perhaps live inside the meta object field? This will definitely make parsing on the Metricbeat end more involved but I do think it's the right place for the field, since it's specific to a particular type of plugin. WDYT?

BTW, similar to the cluster_uuid lookup enhancement you've done for the Elasticsearch output plugin in logstash-plugins/logstash-output-elasticsearch#857, we'll need to enhance the Elasticsearch filter and input plugins as well. This way we can associate the Logstash pipeline/node with any Elasticsearch clusters it touches in the Stack Monitoring UI. I would not consider these further enhancements as a blocker for this PR here.

@ycombinator The parameter to display the graph is now added. Because the key in the return is called graph and because we also refer to the graph key inside the implementation, I went with ?graph=true as the argument to display the full output. Let me know what you think. It is trivial to change it back to representation of course.

The parameter to display the graph is now added.

Thanks @cachedout. I noticed that this parameter works as expected on GET _node/pipleines/{id}. Could we also make it work on GET _node/pipelines? The latter is what Metricbeat actually ended up calling, since it was more efficient to make a single API call and get all pipelines than making an API call per pipeline.

@danhermann Yes, I agree that this approach is sub-optimal, though doing it a different way potentially required some changes to Logstash that I wasn't comfortable making. Are you all right with us getting this in and re-visiting those concerns in a follow-up PR?

Shaunak Kashyap merged this into the following branches!

Hi folks it seems this change introduces a new warning during startup:

[2019-05-06T15:20:52,142][WARN ][org.logstash.instrument.metrics.gauge.LazyDelegatingGauge] A gauge metric of an unknown type (org.jruby.specialized.RubyArrayOneObject) has been create for key: cluster_uuids. This may result in invalid serialization.  It is recommended to log an issue to the responsible developer/development team.

@danhermann should we add the Ruby Array to LazyDelegatingGauge.java?

The Ruby types that are currently wrapped for metrics (RubyHash, RubyTimestamp, etc.) are deprecated for a variety of reasons, so I'm not sure that we want to add another Ruby type. It's possible that we could coerce it into a Java type.

@danhermann since this is a new metric, should we just replace it with a java type then before it ships in a release?

@jsvd, that's what I would suggest.